chore(auth): render the Kerberos/LDAP diagnostic pages with html/template (M39) - #64
Conversation
ef2b9ad to
5c6bac8
Compare
CodeQL note — the flagged alerts are pre-existing patterns, re-attributedCodeQL reports "new alerts in code changed by this pull request". These are alerts that already exist on Baseline on Per rule:
Happy to dismiss them individually, or — probably more useful — treat the 52 🤖 Generated with Claude Code |
5c6bac8 to
ccfad98
Compare
dac58bc to
009fff8
Compare
classifyUser tested u.PasswordHash != "" BEFORE the directory signal, so any
AD-backed user who also carried a local hash migrated as an app-LOCAL user keyed
by that credential. Three things went wrong at once:
- the central ended up holding a STANDING PASSWORD for someone whose identity
is directory-governed, so the migrated account outlived AD-side disablement,
lockout and password policy — exactly what this package's doc comment
promises never happens ("no record or password is copied");
- Classify never BLOCKS a local user, so an AD population misclassified this
way sailed straight past the "central is on a different AD" guard that exists
to stop unauthenticatable users being imported;
- Apply flipped allow_local_users on the target for accounts that must never
use the app-local login path.
Composes with SA-7: that bug was one way an AD user acquired a local hash in the
first place. Fixing SA-7 reduces the population going forward but neither
eliminates it (master-admin break-glass stays legitimate) nor cleans existing
data, so this inversion is independently necessary.
Fix: resolve the directory key first; only a user with NO directory identity is
local. A KindAD entry no longer carries a password hash — the central re-binds
that person from the same AD, so copying the credential would recreate the very
shadow account this prevents. A HadLocalPassword BOOLEAN (never the hash) travels
instead so the dry run can tell the operator which break-glass logins do not
survive the move.
directoryKey prefers SAMAccountName but falls back to ldap/kerberos mappings,
because handleImportLDAPUsers provisions a user with an ldap mapping and NO
SAMAccountName until first login — without the fallback such a user is
misclassified as local or dropped entirely. The `local` provider is deliberately
not consulted: every LDAP/Kerberos JIT provision writes BOTH an ldap and a local
mapping, which is exactly how localUsername happily returned an AD username under
the old precedence.
Selection is deterministic across backends: GetMappingsForUser returns insertion
order on Bolt and UNORDERED rows on Postgres, and a user commonly carries both a
UPN and a sAMAccountName form. The bare form wins, ties break lexicographically,
so both backends produce an identical bundle.
OwnerAppID != "" short-circuits to local: an app-local user is local by
construction and must never be resolved against the directory.
Note on the two directory predicates: handler.isDirectoryBacked (SA-7) is
deny-by-default; migrate.directoryKey is an allow-list of ldap/kerberos. The
asymmetry is INTENTIONAL — one returns a verdict, the other must return a key.
Documented in both places; do not unify (internal/migrate importing from
internal/handler would invert the dependency direction).
WIRE BREAK: SchemaRev 1 -> 2. Classification happens on the SOURCE side, so a
patched central cannot trust the Kind values in a rev-1 bundle. guardMigrationCall
exact-matches the rev, so this surfaces as "upgrade the older deployment first"
rather than a silent import of buggy classification. In-flight migrations must
upgrade the source first — release notes.
Also fixed: dishonest preflight counts. Apply skips a user with no effective
roles entirely while Classify counted them as migrating, so preflight promised N
and Apply delivered fewer with nothing explaining the gap. Report.NoRoles counts
them separately with a note. The AD block checks still run for them — "the
central cannot authenticate this person" is worth saying regardless of roles.
Tests (internal/migrate/classify_test.go): directory user with a local hash ->
AD, no hash carried, HadLocalPassword set; imported LDAP user with no
SAMAccountName still directory; genuine local user unchanged and keeps their
hash; app-local user never directory even with a stray SAMAccountName;
directoryKey order-independent across insertion orders; NoRoles accounting
matches Apply. The first two fail with the precedence reverted. Full suite green.
Stacked on the H15 audience fix — same functions, same hunks.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…late (M39) CodeQL reported 10 open go/reflected-xss alerts in internal/handler/auth.go — the last of the class after bodaay#58 retired the oidc.go and hosted_login.go ones. Unlike those, which were the sanitizer-not-recognised false positive with every value already html.EscapeString-wrapped, THREE of these sites had no escaping at all: fmt.Fprintf(w, h.bp(negotiateTestKrbFailedHTML), "Invalid SPNEGO token: "+err.Error()) fmt.Fprintf(w, h.bp(negotiateTestKrbFailedHTML), "Kerberos ticket could not be parsed: "+err.Error()) err there is the result of parsing tokenBytes — the base64 payload of the caller's `Authorization: Negotiate` header. A fourth, adjacent site DID escape (html.EscapeString(err.Error())), which is what makes the omission a slip rather than a policy. The success page was worse in breadth: it reflected every AD attribute — displayName, mail, department, company, title, memberOf — raw into an HTML table. Those are attacker-influenced for any principal who can edit their own directory record. Exploitability is bounded and stated honestly: both endpoints register only when AUTH_ENABLE_TEST_ENDPOINTS=true, which defaults OFF and was already gated for exactly this reason under H1 (they perform live LDAP binds and are a password oracle). Whether a crafted SPNEGO token can drive '<' into a gokrb5 ASN.1 error string was NOT established — it depends on that library's error formatting. So: a genuine unescaped reflection of attacker-derived data, on a default-off endpoint, with uncertain end-to-end exploitability. Fixed on the merits rather than argued about. Approach: the same conversion bodaay#58 applied to the OIDC login page. Six Fprintf templates become html/template with a typed negotiateTestData struct, parsed once at package init via template.Must. {{BASE_PATH}} — which h.bp() substituted with strings.ReplaceAll BEFORE the Fprintf — becomes a real {{.BasePath}} field, so the base path is escaped for its context too and the bp() hop disappears on these paths. All manual html.EscapeString calls here are removed; the template owns escaping. One site deliberately NOT converted: the SPNEGO retry meta-refresh (<meta http-equiv="refresh" content="0;url=...">). html/template classifies <meta content> as contentTypeUnsafe — it attribute-escapes but does NOT URL-filter — so a rewrite would add no guarantee. retryURL is built server-side from h.url() plus url.QueryEscape'd values and is never echoed from the request; the EscapeString there is defence in depth. Recorded in a comment so it is not "cleaned up" later. Tests (internal/handler/negotiate_test_pages_test.go): error pages escape a script payload and are SINGLE-encoded (a surviving manual EscapeString would show as &lt;); the success page escapes AD-controlled attributes; every form page renders a real base path with no placeholder left behind; and the wait page still returns 401, which the SPNEGO handshake depends on — the conversion moved WriteHeader into a shared helper, exactly the kind of thing a refactor drops silently. Full suite green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
009fff8 to
a994b11
Compare
Last of six. Stacked on #63. Retires the final
go/reflected-xssalerts — CodeQL on master goes to zero for this rule.Not the same as #58
#58's alerts were the sanitizer-not-recognised false positive: every value was already
html.EscapeString-wrapped. These are not. Two sites passederr.Error()with no escaping at all:errthere is the result of parsingtokenBytes— the base64 payload of the caller'sAuthorization: Negotiateheader. An adjacent site did escape (html.EscapeString(err.Error())), which makes the omission a slip rather than a policy.The success page was worse in breadth: it reflected every AD attribute —
displayName,mail,department,company,title,memberOf— raw into an HTML table. Those are attacker-influenced for any principal who can edit their own directory record.Exploitability, stated honestly: both endpoints register only when
AUTH_ENABLE_TEST_ENDPOINTS=true, which defaults off and was already gated for exactly this reason under H1. Whether a crafted SPNEGO token can drive<into a gokrb5 ASN.1 error string was not established — it depends on that library's error formatting. So: a genuine unescaped reflection of attacker-derived data, on a default-off endpoint, with uncertain end-to-end exploitability. Fixed on the merits rather than argued about.Approach
The same conversion #58 applied to the OIDC login page: six
fmt.Fprintftemplates becomehtml/templatewith a typed struct, parsed once at package init.{{BASE_PATH}}— whichh.bp()substituted withstrings.ReplaceAllbefore theFprintf— becomes a real{{.BasePath}}field, so the base path is escaped for its context too.A rendering bug an adversarial review caught
negotiateTestCSSstill carriedfmt-escaped%%(border-radius:50%%,width:100%%).html/templateis not a format string, so those would have shipped literally and broken every rule containing them — including on the three pages that already usedfmt.Fprint(no formatting) and were therefore silently broken before this change too. Un-doubled, and the test asserts no%%survives into any rendered page.One site deliberately not converted
The SPNEGO retry meta-refresh (
<meta http-equiv="refresh" content="0;url=…">).html/templateclassifies<meta content>ascontentTypeUnsafe— it attribute-escapes but does not URL-filter — so a rewrite would add no guarantee. The reasoning is recorded in a comment so it isn't "cleaned up" later.Tests
Error pages escape a script payload and are single-encoded (a surviving manual
EscapeStringwould show as&lt;); the success page escapes AD-controlled attributes; every form page renders a real base path with no placeholder left behind; and the wait page still returns 401, which the SPNEGO handshake depends on — the conversion movedWriteHeaderinto a shared helper, exactly the kind of thing a refactor drops silently.🤖 Generated with Claude Code